test(tbtc): add reservation proposal marshaling coverage - #4277

…oordination

Implementation-plan.md Milestone 3, 'multi-signer simulated
integration test' item (per user decision: build the test, leave the
testnet-drill item as an agent-not-actionable tracked item since it
needs live infra and calendar time, not code).

Scales TestCoordinationExecutor_Coordinate's existing 3-operator
harness - deterministic keypairs, real per-operator localChain fakes,
a real shared netlocal.BroadcastChannel, one goroutine per operator
running coordinationExecutor.coordinate concurrently - to
ReservationAnchorProposal and ReservationReanchorProposal. This
exercises the real leader/follower coordination round-trip
(checklist generation -> leader election -> broadcast -> follower
validation -> convergence) that no mocked pkg/tbtcpg unit test can
cover, since those call task.Run(request) directly and never go
through coordinationExecutor.coordinate. It also exercises PR
#4277's protobuf marshaling of both proposal types over a real wire
round-trip, since every follower unmarshals the leader's broadcast
coordinationMessage.

Depends on PR #4278 (this branch's parent): before that fix,
ActionReservationAnchor/ActionReservationReanchor never appeared in
getActionsChecklist's output, so every operator's checklist search in
these tests would fall through to NoopProposal and fail - confirmed
by temporarily reverting the checklist fix and re-running (both new
tests failed with the expected NoopProposal mismatch), then restoring
it.

Found and fixed one bug in this test's own harness during
verification: both new tests initially shared one netlocal broadcast
channel name. getBroadcastChannel's registry is keyed by name and
never releases old channels, so under -race (which changed
goroutine/channel-delivery timing enough to surface it in ~every
run), the reanchor test's follower sometimes received a stale
broadcast left over from the anchor test's leader. Fixed by giving
each test its own channel name; re-verified stable across 10
repeated -race runs plus the full non-race and race suites.

Testing:
- go test ./pkg/tbtc/...: 365/365 pass.
- go test -race ./pkg/tbtc/...: clean, no data races, including
  -count=10 on just the two new tests.
- go build ./... && go test ./...: full repo, 49 packages, zero FAIL.
- gofmt -l / go vet: clean.

…oordination (#4279)

## Summary

Implements `implementation-plan.md` Milestone 3's "multi-signer
simulated
integration test" item - the last piece of the full M1
keep-core-readiness
implementation plan (M0 is an external release-coordination gate, not a
code
task; M1's four rows and M2's test-coverage backfill are covered by
[#4276](#4276),
[#4277](#4277),
[#4278](#4278), and a
separate M2 follow-up PR).

Stacked on `m1/reservation-coordination-checklist`
([#4278](#4278)).

**Scope note (per explicit decision this session):** Milestone 3 has two
items - this test, and a "testnet round with a forced liveness/stranding
drill" (~2 weeks, needs a live testnet deployment and real
multi-operator
wall-clock timing). Only the former is code; the latter is tracked as an
agent-not-actionable item in the plan doc, unchanged by this PR.

## Change

Scales `TestCoordinationExecutor_Coordinate`'s existing 3-operator
harness -
deterministic keypairs, real per-operator `localChain` fakes, a real
shared
`netlocal.BroadcastChannel`, one goroutine per operator running
`coordinationExecutor.coordinate` concurrently - to
`ReservationAnchorProposal`
and `ReservationReanchorProposal`, added as one table-driven test with
`anchor`/`reanchor` subtests:

- `TestCoordinationExecutor_Coordinate_ReservationProposals`

This exercises the real leader/follower coordination round-trip
(checklist
generation -> leader election -> broadcast -> follower validation ->
convergence) that no mocked `pkg/tbtcpg` unit test can cover, since
those
call `task.Run(request)` directly and never go through
`coordinationExecutor.coordinate`. It also exercises #4277's protobuf
marshaling of both proposal types over a real wire round-trip, since
every
follower unmarshals the leader's broadcast `coordinationMessage`.

**Depends on #4278** (this branch's parent): before that fix,
`ActionReservationAnchor`/`ActionReservationReanchor` never appeared in
`getActionsChecklist`'s output, so every operator's checklist search in
these tests fell through to `NoopProposal` and failed. Verified
directly:
temporarily reverted #4278's checklist change, re-ran the new test (both
subtests failed with the expected `NoopProposal` mismatch), then
restored it.

## A bug found in this test's own harness, and its root-cause fix

The two reservation subtests initially shared one `netlocal` broadcast
channel name. `getBroadcastChannel`'s registry is keyed by name, is
process-global, and never released old channels' retransmission tickers
(they were wired to `context.Background()`), so under `-race` the
reanchor
subtest's follower sometimes received a stale broadcast left over from
the
anchor subtest's leader - a cross-test data race in the test harness
itself, not in the production code under test.

Root-caused and fixed in `pkg/net/local` (production, non-test code,
since
the registry it fixes is used by every test file that exercises a
simulated
local network): each broadcast channel's retransmission ticker context
is
now cancellable, and a new `ReleaseBroadcastChannel(name string)`
cancels
and de-registers a channel's own ticker(s) by name (scoped to the
caller's
own channel, not a global reset) on `t.Cleanup`. This is now wired into
all
four broadcast-channel-creation sites in `pkg/tbtc/coordination_test.go`
(the shared operator helper plus three pre-existing hardcoded-name
tests),
each releasing under its own channel name.

## Testing

- `go build ./...`, `go vet ./...`, `gofmt -l`: clean.
- `go test ./pkg/tbtc/...` and `go test -race ./pkg/tbtc/...`: full
suite
  green, including `-count=10` targeted at the new/changed coordination
  tests.
- `go test ./pkg/net/local/... ./pkg/net/retransmission/...` (incl.
`-race`):
  green, including new coverage for `ReleaseBroadcastChannel`'s actual
effect (a released channel's ticker stops retransmitting; releasing and
  reopening under the same name only delivers to the new registration).

## Not in this PR

- The testnet-round liveness/stranding drill (Milestone 3's other item)
-
  operational, not code; tracked separately.
- Milestone 2's test-coverage backfill - separate follow-up PR.

…nt review of #4282) (#4283)

## Summary

Remediation for the 37 confirmed findings from a multi-agent review of
PR #4282
(`dev` <- `reservations-epic`, i.e. the accumulated content of
#4274+#4276+#4277).
37 raised -> 37 confirmed -> 0 dropped after arbitration and validation.

- **P1 (6 of 7 fully fixed, 1 partially fixed):** deposit-sweep
reservation-vault
exclusion, reservation look-back underflow + target-wallet check,
reservation
acceptance `eth_getLogs` bounds + nonce reconciliation + caps, SPV
proof-loop
retry-eviction data loss (symptom fixed, structural root cause deferred
- see
below), stale-deposit timeout memoization, below-dust re-anchor trigger
removal
  (M-27, resolved via tbtc-v2 source after user escalation).
- **P2/P3 (22 of 30 fixed, 8 explicitly deferred):** see "Deferred"
below.

Full-repo `go build`, `go vet`, and `go test ./...` all pass with these
fixes
applied (verified after every commit and once more at closeout).

## Deferred (1 P1 architectural root-cause + 7 P2/P3 symptoms/hygiene)

An arbiter-recommended structural fix for M-16 (remove the SPV proof
loop's
persistent-cursor design entirely in favor of the stateless
bounded-rescan
pattern every sibling proof type already uses) was attempted together
with
the M-7 nonce-aware timeout fix and an M-14 dead-code removal. That
combined
change broke three existing tests and was reverted rather than debugged
under
time pressure. Only a narrower, independently-safe subset landed: a
surgical
patch for M-3 (non-lossy cursor rewind) plus unrelated
memoization/metrics/test
fixes. **M-16's own P1 rating is only partially addressed** - the
persistent-cursor
design itself, and the M-7/M-14 symptoms it also breeds, remain
unremoved.

1. **M-16 (P1)** `pkg/maintainer/spv/reservation_proof_loop.go:227-246`
-
`reservationProofScanState`'s persistent cursor is the structural root
cause of M-3 (fixed surgically) and M-7 (below). Removing it in favor of
   the stateless bounded-rescan pattern is what broke 3 tests on first
   attempt and remains unimplemented.
2. **M-7 (P2)** `reservation_action_timeout_watch.go:260-281` -
`CheckReservationActionTimeouts` deletes `pendingActions` entries on 3
of
   4 non-notifying outcomes without asserting the tracked `requestNonce`
   against the freshly-derived one; same root cause as M-3.
3. **P2** `reservation_action_timeout_watch.go:370` +
`reservation_wiring.go:38-49` -
the timeout watcher's `WalletMembersResolver` only resolves wallets the
   local operator co-signs; an offline/disabled/colluding wallet's own
   operators get zero independent timeout coverage.
4. **P2** dead-code cluster in `reservation_proof_loop.go` /
`reservation_proof_loop_test.go` -
`findReservationAcceptanceTransaction`,
`findReservationReanchorTransaction`, and their wrapper helpers have
zero
production callers; 14 tests exercise the unused wrapper instead of the
   `isMatching*` predicates actually called in production.
5. **P2** `reservation_proof_loop.go:612,~817` - two tautological guards
are
algebraically always-false, masking that the real enforced constraint is
   only `0 < fee <= TxMaxFee`.
6. **P2** `reservation_wiring.go:237-320` `startStaleDepositPoll` - the
entire loop body runs untested inside a goroutine; existing tests assert
   only that the goroutine starts.
7. **P3** `reservation_action_timeout_watch.go:18-20` - unused
   "backward-compatibility alias" constant, zero references.
8. **P3** `reservation_proof_loop.go:644` - duplicated, truncated
comment
   fragment left by a merge.

## Known conflicts with other open PRs in this stack - read before
merging

This branched from `reservations-epic` at `bb3dcb398`. Three other
efforts are
in flight against overlapping code and were **not** reconciled here,
since they
belong to PRs this one doesn't own:

### 1. `pkg/tbtc/coordination.go` vs #4278 (hard conflict, not cosmetic)

#4278 ("remove frequency gate on reservation checklist actions") drops
`&& windowIndex%frequencyWindows == 0` from the reservation-actions
checklist
gate (custody-critical, should run every window like `ActionRedemption`)
but
its diff still references the old single `ReservationsActivationBlock`
constant. This PR's `602d0ef11` independently rewrote that same `if`
into
`reservationsActivationBlock(ce.ethereumNetwork)`, a per-network table
lookup
(`ethereum.Mainnet: 26500000`, everything else defaults to 0).

**A conflict resolution that naively favors this PR's side of that hunk
silently reinstates the frequency gate #4278 deliberately removed.**
Combined
resolution (verified against both intents):

```go
// Reservation actions (acceptance, re-anchor) are custody-critical like
// Redemption and are checked on every coordination window once the
// activation block is reached, not frequency-gated like the
// throughput-driven DepositSweep/MovedFundsSweep/MovingFunds actions
// above: a delayed reservation acceptance or re-anchor risks the
// on-chain ReservationActionTimeout backstop firing before the wallet
// subsystem gets a chance to act. The activation block is a per-network
// table (reservationsActivationBlock), not a single global constant, but
// it is still config-independent and globally observable from chain
// height alone -- which is what keeps leader and follower checklists in
// agreement without relying on local config.
if coordinationBlock >= reservationsActivationBlock(ce.ethereumNetwork) {
    actions = append(actions, ActionReservationAnchor)
    actions = append(actions, ActionReservationReanchor)
}
```

### 2. `pkg/tbtc/marshaling.go` vs #4278 (duplicate, this PR's version
wins)

#4278 independently adds the same 4 missing `Marshal`/`Unmarshal` doc
comments
this PR's `7cbb8cc2f` adds, but comment-only and with a capitalization
bug
(lowercases the exported type name, e.g. `"...converts the
reservationAnchorProposal..."`). This PR's version is a superset:
correctly
capitalized comments plus the actual nil-guard/zero-hash-rejection logic
#4278 doesn't have. On merge, take this PR's 4 lines, drop #4278's.

### 3. `pkg/tbtcpg/reservation_acceptance_test.go` vs #4280 (whole-file
conflict + one real design decision)

#4280 ("M2 test-coverage backfill") independently rewrote large parts of
the
same shared test harness this PR's `726f05ed7` touched - the same
`reservationAcceptanceLocalChain` type, constructor, and ~14 shared
methods,
plus `scenarioReservationAcceptanceChain`/`registerReservedDeposits`/
`expectedAnchorsEqual`. This is a heavy line-level conflict across the
whole
file, not just redundant test names. Specifics:

- `TestReservationAcceptanceTask_AmountCapBoundaries` (this PR, cap
boundaries
only) is a strict subset of #4280's
`TestReservationAcceptanceTask_BoundaryChecks`
  (adds `MaxReservationsPerWallet`, net-of-fee `ReservationMinAmount`,
`ActiveReservationsCount`). Left in place rather than deleted
preemptively -
#4280 is still open and two-deep-stacked (on #4278, also open) and could
  stall or be reworked; delete this PR's version only in the merge that
  actually lands #4280.
- This PR's
`TestReservationAcceptanceTask_VaultNotConfigured_ZeroAddress`
(finding: dead vault-not-configured guard) has no equivalent on #4280's
  side - a "just take #4280's file" resolution silently drops it.
- **Real design decision, not just a merge conflict:** #4280's
  `TestReservationAcceptanceTask_Stateless_PastEventsError` exercises
`PastReservationAcceptanceRequestedEvents` returning an error and
asserts
fail-closed skip-on-error. This PR's `hasPendingAction` (from
`726f05ed7`)
no longer calls `PastReservationAcceptanceRequestedEvents` at all - it
uses
a different, generation-scoped pending-action check instead. Ported onto
this PR's code as-is, that test would either pass vacuously or fail for
an
unrelated reason. **This PR intentionally left
`PastReservationAcceptanceRequestedEvents`
  on the `tbtcpg.Chain` interface (`chain.go:263`) and the test double's
`acceptanceEvents`/`acceptanceEventsErr` fields in place, undeleted,
even
though they now have zero production callers** - removing them here
would
have foreclosed reconciling #4280's test against whichever
pending-action
mechanism is ultimately kept. Whoever merges this PR and #4280 needs to
pick one mechanism and either delete the losing side's interface
method/test
  or keep both if there's a reason for two independent checks.

## Testing

- `go build ./...`, `go vet ./...`: clean.
- `go test ./...`: full repo suite, 0 failures (verified at closeout
after
  every commit landed).